Honour the remember-decision duration when a request is refused - #476
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughNIP-46 rejection handling now forwards the selected ChangesNIP-46 rejection duration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Nip46ApprovalScreen
participant Nip46ApprovalActivity
participant respond
Nip46ApprovalScreen->>Nip46ApprovalActivity: reject(selectedDuration)
Nip46ApprovalActivity->>respond: respond(rejection, selectedDuration)
respond-->>Nip46ApprovalActivity: mapped PermissionDuration response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalScreen.kt`:
- Around line 198-203: Update the rejection handler in Nip46ApprovalScreen so
onClick passes PermissionDuration.JUST_THIS_TIME when isConnectRequest is true,
while preserving selectedDuration for other refusals. Add a regression test
covering a connect-request refusal and verifying the one-shot duration is
recorded.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9d4ab1b-1acf-45c3-b6e2-7abe5ae16ee2
📒 Files selected for processing (3)
app/src/androidTest/kotlin/io/privkey/keep/nip46/Nip46RejectDurationTest.ktapp/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalActivity.ktapp/src/main/kotlin/io/privkey/keep/nip46/Nip46ApprovalScreen.kt
|
Taken, and worth being precise about why, because the current behaviour was already safe. A connect request hides the selector and defaults it to Forever, so forwarding that value records a decision from a control the user never saw. Nothing is stored today, because Forever yields no window and the signer only remembers bounded ones. So the outcome is correct and the reasoning is not: the safety lives in another crate, one release away from changing. That coupling is the actual problem. Permanent refusals were deliberately left unsupported, on the grounds that an in-memory refusal claiming to last forever would quietly vanish on restart. If a later change adds persistence and honours Forever, this line silently converts a hidden default into a permanent block, and nothing here would fail. Passing the one-shot value explicitly makes the property local instead of borrowed. A regression test covers it, asserting a connect refusal reports the one-shot value rather than the hidden default. Also confirmed on the emulator in the previous run: the negative case passes, so refusing without touching the selector records nothing, and the positive case passes, so a chosen duration does reach the callback. That second one had failed earlier for a reason unrelated to the feature: the test clicked the label to open the menu, but the control is an exposed dropdown whose menu opens from the field showing the current selection. My assumption about the interaction, not the wiring. |
1 participant
Summary
The approval screen has a duration selector labelled "Remember this decision". A refusal is a decision, and the selector's value was passed only to approve; on the reject branch it was discarded and replaced with the one-shot value. So a user could choose "for 1 hour", refuse, and be asked again on the very next request.
That was invisible until the signer learned to remember refusals. Now that it does, the value has somewhere to go: a refusal carrying a duration is recorded and later requests of the same kind are refused without prompting, which is what stops a client that retries on failure from re-asking indefinitely. This is the surface half of that work; the core landed separately.
Two properties make this safe rather than a new way to lock yourself out.
The default records nothing. A signing request defaults the selector to the one-shot value, which yields no window, so an ordinary refusal behaves exactly as before. Only a duration the user deliberately chose is remembered.
Every internal refusal passes no duration, and there are several: the kill switch, a failed biometric, an unavailable cipher, a back press. Those map to the one-shot value and record nothing, which is correct, since none of them is the user deciding anything about this app.
The shape follows a reference signer for the same protocol, which hands one shared remember control to both its accept and reject callbacks. The control means "remember my decision for this long", and the decision is whichever button was pressed. Keep's label already said that; only the code disagreed.
Test plan
Two instrumented tests on the screen, pinning the wiring rather than the rendering, since what the reject button hands back is what decides whether the refusal is remembered.
The first is the negative and matters more: refusing without touching the selector must report the one-shot value. If reject forwarded anything else, every ordinary "no" would silently become a lasting block nobody asked for. The second chooses an hour, refuses, and requires that duration to reach the callback, so the selector is not decorative on that branch.
Compiles locally against freshly generated bindings. These need real Compose, so they run on the emulator in CI.
Summary by CodeRabbit
New Features
Tests